Skip to content

Conversation

vizanto
Copy link

@vizanto vizanto commented Nov 18, 2015

Instead of just complaining, I'd figure I make a pr :-)

@weavejester
Copy link
Member

Thanks for the patch. However, I'm uncertain of the use case. Under what circumstances would this be useful? That is, under what scenarios could the nested-params middleware legitimately be applied more than once?

Second, you use metadata, but you attach the metadata before compilation to the following form and not, as I suspect you intended, to the returned parameter map. You'd need to use the with-meta function for your patch to work.

Third, you have no tests for this change. Tests protect against regressions and allow me to see your patch is working. It also helps you see your own patch is working!

Fourth, the "more precise doc-strings" commit isn't directly related to this PR, so it should be in a separate PR.

Fifth, you assume that the metadata for the parameters remains, but operations on data structures will frequently clear metadata. This makes it an unreliable mechanism for checking if the request map has had the nested parameters middleware applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants